Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds multi-threaded job processing capabilities to the cyhy_commander application. The changes enable parallel processing of completed scan jobs using configurable thread pools and add thread-aware logging throughout the system.
- Introduces configurable multi-threading for processing completed jobs with Queue-based job distribution
- Adds thread name context to all log messages for better traceability in concurrent execution
- Implements queue monitoring thread to track processing metrics
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cyhy_commander/commander.py | Implements multi-threaded job processing with queues, thread pool management, and synchronization mechanisms |
| cyhy_commander/nmap/nmap_importer.py | Adds thread-aware logging with thread name prefixes to all log statements |
| cyhy_commander/nessus/nessus_importer.py | Adds thread-aware logging with thread name prefixes to all log statements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mcdonnnj
added
the
improvement
dav3r
left a comment
dav3r
left a comment
There was a problem hiding this comment.
This all looks great to me. 👍 🧵 🗒️ ⏱️
jsf9k
left a comment
jsf9k
left a comment
There was a problem hiding this comment.
I did a quick review of this last night and it looks good to me so far. I'll do a more careful review once you're ready to take it out of draft.
It's a great idea to add some stuff like this, so we can actually get a feeling for what is going on during scanning and where the trouble spots are.
This updated the `process_job_from_queue()` inner function to provide the duration it took to process a job (or discover the queue was empty) and we then output the job processing duration if a job was processed.
Output to the debug log when starting and ending processing,
Add some basic bracketing messages and general information about what the importer is doing in its callback methods.
As a result of testing add some additional logging, adjust the message for a log message, and ensure that logging is moved out of loops and remains high level.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mcdonnnj
force-pushed
the
improvement/add_performance_information_for_job_processing
branch
from
c8f5599 to
2ff655e
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.__ticket_manager.ips.add(ip_line) | ||
| self.__logger.debug( | ||
| "[%s] Found %d targets in target file %s" | ||
| % (thread_name, len(self.__ticket_manager.ips), target_filename) | ||
| ) |
There was a problem hiding this comment.
This code will fail when stage == STAGE.BASESCAN because self.__ticket_manager is set to None in that case (line 75 in __init__). Attempting to access self.__ticket_manager.ips will raise an AttributeError.
Consider checking if self.__ticket_manager is not None before accessing its attributes, or restructuring the code to handle the BASESCAN case differently.
There was a problem hiding this comment.
I believe the BASESCAN stage is deprecated, though has never been fully removed from the code.
There was a problem hiding this comment.
That's what I thought when I noticed this exact problem as I implemented this code. How do we want to proceed?
There was a problem hiding this comment.
I'm ok ignoring this for now, since we don't actually use the BASESCAN stage. If you want to add a comment or a TODO issue to remove BASESCAN, feel free.
| self.__ticket_manager.close_tickets() | ||
| self.__logger.debug( | ||
| "[%s] Clear the latest flag for applicable VulnScanDocs" % thread_name | ||
| ) | ||
| self.__ticket_manager.clear_vuln_latest_flags() |
There was a problem hiding this comment.
This code will fail when stage == STAGE.BASESCAN because self.__ticket_manager is set to None in that case (line 75 in __init__). Attempting to call self.__ticket_manager.close_tickets() and self.__ticket_manager.clear_vuln_latest_flags() will raise an AttributeError.
Consider checking if self.__ticket_manager is not None before calling its methods.
| self.__ticket_manager.close_tickets() | |
| self.__logger.debug( | |
| "[%s] Clear the latest flag for applicable VulnScanDocs" % thread_name | |
| ) | |
| self.__ticket_manager.clear_vuln_latest_flags() | |
| if self.__ticket_manager is not None: | |
| self.__ticket_manager.close_tickets() | |
| self.__logger.debug( | |
| "[%s] Clear the latest flag for applicable VulnScanDocs" % thread_name | |
| ) | |
| self.__ticket_manager.clear_vuln_latest_flags() |
There was a problem hiding this comment.
I believe the BASESCAN stage is deprecated, though has never been fully removed from the code.
There was a problem hiding this comment.
That's what I thought when I noticed this exact problem as I implemented this code. How do we want to proceed?
There was a problem hiding this comment.
I'm ok ignoring this for now, since we don't actually use the BASESCAN stage. If you want to add a comment or a TODO issue to remove BASESCAN, feel free.
🗣 Description
This pull request adds logging to indicate how long a job took to process and insight into what the Nmap-based job (BASESCAN, NETSCAN1, NETSCAN2, PORTSCAN) is doing while being processed.
💭 Motivation and context
While testing for #15 it became apparent that we wanted to know how long individual jobs took to process and that we wanted insight into where an Nmap-based job was in its processing.
🧪 Testing
This has been working ok in @dav3r's test environment.
✅ Pre-approval checklist